Introduce `JUnitClassModifiers` check #214

oxkitsune · 2022-08-24T08:08:43Z

Implement BugPattern to ensure JUnit test classes are declared as package private final.

Example:

-public class A {
-  @Test
-  void foo () {}
-}

+final class A {
+  @Test
+  void foo () {}
+}

Suggested commit message:

Introduce `JUnitClassModifiers` check (#214)

rickie

Added a commit.

We need to think about nested classes. What if that occurs, what are the rules w.r.t. modifiers. Additionally, we need a test case for that 😉.

Maybe we could to a test run on an internal codebase to see how it performs 😄.

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitClassDeclaration.java

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/MoreMatchers.java

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitClassDeclaration.java

...r-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitMethodDeclaration.java

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/MoreMatchers.java

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/StaticImport.java

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/MoreMatchers.java

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitClassDeclaration.java

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/MoreMatchers.java

rickie

Added a commit.

Also asked a few questions :).

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitClassDeclaration.java

rickie · 2022-10-25T09:22:06Z

...rone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/JUnitClassDeclarationTest.java

+            "",
+            "// BUG: Diagnostic contains:",
+            "class B {",
+            "  @ParameterizedTest",


Is there a specific reason to use @ParameterizedTest? To also account for that case?

Yes, we check whether the class contains methods annotated with @Test or with meta annotation org.junit.jupiter.api.TestTemplate. @ParameterizedTest is annotated with @TestTemplate and this test ensures we also match tests like this.

rickie

Added a commit.

I think this is a really nice improvement. Would love to see this applied on our internal codebase :).

I found some edge cases that we should consider:

If something is abstract, it cannot be final. So we need to exclude that and add a test case for that :).
In our shared modules we have some public abstract classes with tests that can be used by downstream projects. These classes are not located in /src/test/ and marked public on purpose. I think this use case is rather specific to our shared modules repository, but IMO we should do something with it. On the other hand, I'm not sure if looking at the file's location is something we should add as heuristic for flagging these things.

rickie · 2022-11-01T09:44:46Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitClassModifiers.java

+    tags = FRAGILE_CODE)
+public final class JUnitClassModifiers extends BugChecker implements ClassTreeMatcher {
+  private static final long serialVersionUID = 1L;
+  private static final MultiMatcher<MethodTree, AnnotationTree> TEST_METHOD =


Since we are re-using some of those, should we maybe move them to MoreMatchers? I discussed this with @eric-picnic already and he made a setup in #319. Although there we have a specific JUnit util.

I think we can for now roll with this and have the bigger discussion in #319 as we have some more things that we plan to extract to utils there. So I'd say let's keep this here for now.

There is a https://github.com/google/error-prone/blob/master/check_api/src/main/java/com/google/errorprone/matchers/JUnitMatchers.java that sadly enough only supports JUnit lower than 5... Ideally we contribute improvements upstream, but I'm not sure that will work...

rickie

Another thought :).

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitClassModifiers.java

oxkitsune · 2022-11-11T13:06:06Z

Our MoreMatchers#hasMetaAnnotation(String) method cannot match Spring's @Configuration as meta annotation.
I suspect this has something to do with the fact that @Configuration is not explicitly @Inherited.
I've implemented a quick work around that flags some often used @Configuration annotations for now.

rickie

Reverted the introduction of MoreMatchers as this will be part of #335.

This means we'll temporarily introduce two duplicate methods hasMetaAnnotation and hasAnnotation. However, they will soon be de-duplicated by #335.
I seems better to focus this PR solely on introducing the JUnitClassModifiers check.

Suggested commit message (repeating PR title):

Introduce `JUnitClassModifiers` check (#214)

Really cool check @oxkitsune , this will have many impact on other teams 🚀 !

rickie · 2022-11-18T11:19:55Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitClassModifiers.java

+      allOf(
+          not(hasMetaAnnotation("org.springframework.context.annotation.Configuration")),
+          not(
+              hasMetaAnnotation(


We can combine these three not(hasMetaAnnotation( by wrapping them in a not(anyOf(.

rickie

One open question from my side left.

rickie · 2022-11-18T11:36:29Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitClassModifiers.java

+    linkType = CUSTOM,
+    link = BUG_PATTERNS_BASE_URL + "JUnitClassDeclaration",
+    severity = WARNING,
+    tags = FRAGILE_CODE)


Suggested change

tags = FRAGILE_CODE)

tags = SIMPLIFICATION)

Maybe this would fit better? Or STYLE?

Pushing something as it certainly does not match the description of FRAGILE_CODE.

rickie

Re-approving the latest changes.

Have one thing to ponder, in RefasterRuleModifiersTest we give more descriptive names to the classes. This makes it easier to see what exact case is being tested. We could decide to do the same here. Already liking the existing setup, so approving regardless.

Stephan202

Rebased and added a commit. Only open point from my side is the @Configuration case. Feel free to send me example classes over Slack.

Updated the suggested commit message.

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitClassModifiers.java

Stephan202 · 2022-11-27T15:43:40Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitClassModifiers.java

+          not(
+              annotations(
+                  AT_LEAST_ONE,
+                  anyOf(
+                      isType("org.springframework.context.annotation.Configuration"),
+                      hasMetaAnnotation("org.springframework.context.annotation.Configuration")))),


Can we document for which specific cases we need this? (And does this perhaps only prevent the annotated class from being declared final?)

any springboot tests using an annotation marked with @Configuration cannot be marked final. Usually these classes can be package private, so I'm in favour of doing that.

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitClassModifiers.java

...-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/JUnitClassModifiersTest.java

Stephan202 · 2022-11-27T16:03:21Z

...-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/JUnitClassModifiersTest.java

+            "  @Nested",
+            "  // BUG: Diagnostic contains:",
+            "  class Nested1 {",
+            "    @Test",
+            "    void foo() {}",
+            "  }",


The @Nested doesn't influence the checker one way or another 👀

I thought in this case it could be seen as "more realistic"? As the docs of this annotation mentions that it can be used in these kind of cases. OTOH it could also be deleted indeed.

I agree that realism is better, but (IIUC) there's no code in the checker that could possibly be influenced by the annotation. I'm not sure where exactly we should draw the line, though simpler seems preferred 🤔

Stephan202 · 2022-11-27T16:25:42Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitClassModifiers.java

+@BugPattern(
+    summary = "JUnit test classes should be declared as package private final",
+    linkType = CUSTOM,
+    link = BUG_PATTERNS_BASE_URL + "JUnitClassDeclaration",


Suggested change

link = BUG_PATTERNS_BASE_URL + "JUnitClassDeclaration",

link = BUG_PATTERNS_BASE_URL + "JUnitClassModifiers",

github-actions · 2022-12-01T14:19:24Z

Looks good. All 8 mutations in this change were killed.

class	surviving	killed
🎉`tech.picnic.errorprone.bugpatterns.JUnitClassModifiers`	0	8

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

github-actions · 2022-12-01T14:19:28Z

Looks good. All 8 mutations in this change were killed.

class	surviving	killed
🎉`tech.picnic.errorprone.bugpatterns.JUnitClassModifiers`	0	8

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

github-actions · 2022-12-01T15:05:48Z

Looks good. All 9 mutations in this change were killed.

class	surviving	killed
🎉`tech.picnic.errorprone.bugpatterns.JUnitClassModifiers`	0	9

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

rickie

Let's get this one out 😄.

rickie · 2022-12-02T09:24:23Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitClassModifiers.java

+      return Description.NO_MATCH;
+    }
+
+    boolean hasSpringConfiguration = TEST_CLASS_WITH_SPRING_CONFIGURATION.matches(tree, state);


I really like the code. Got one minor point to consider, do we want to name it TEST_CLASS_WITH_SPRING_CONFIGURATION_ANNOTATION? It's a bit long I'd say but WITH_SPRING_CONFIGURATION out of context is not that clear.

Stephan202

Rebased and added one more commit. Repeating the suggested commit message:

Introduce `JUnitClassModifiers` check (#214)

Stephan202 · 2022-12-03T08:55:46Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitClassModifiers.java

+    boolean hasSpringConfiguration = TEST_CLASS_WITH_SPRING_CONFIGURATION.matches(tree, state);
+    if (hasSpringConfiguration && tree.getModifiers().getFlags().isEmpty()) {
+      return Description.NO_MATCH;
+    }


I think this test can be pushed into the TEST_CLASS_WITH_INCORRECT_MODIFIERS matcher; will push a proposal. (Downside is that the matcher would be evaluated twice in case of a hit, but that should rarely happen, since we expect users to clean up their code ;). Plus, it's not an expensive check.)

Stephan202 · 2022-12-03T08:58:18Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitClassModifiers.java

+              hasModifier(Modifier.PROTECTED),
+              hasModifier(Modifier.PUBLIC)));
+
+  private static final MultiMatcher<ClassTree, AnnotationTree>


Suggested change

private static final MultiMatcher<ClassTree, AnnotationTree>

private static final Matcher<ClassTree>

Would also omit the empty line above.

Stephan202 · 2022-12-03T09:00:25Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitClassModifiers.java

+  private static final Matcher<ClassTree> TEST_CLASS_WITH_INCORRECT_MODIFIERS =
+      allOf(
+          hasMethod(TEST_METHOD),
+          not(hasModifier(Modifier.ABSTRACT)),


Let's move this case down so that the modifier ordering here matches the resolution order below.

^ I'll shuffle the test code accordingly.

Stephan202 · 2022-12-03T09:19:15Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitClassModifiers.java

+    if (!hasSpringConfiguration) {
+      SuggestedFixes.addModifiers(tree, state, Modifier.FINAL).ifPresent(fixBuilder::merge);
+    }


Pitest doesn't flag it because we're not using the STRONGER mutators group (I'll fix that), but right now the tests also pass if this suggested fix is applied unconditionally. We need to extend the replacement test as well.

I'll fix that

I filed #383.

github-actions · 2022-12-03T09:27:48Z

Looks good. All 6 mutations in this change were killed.

class	surviving	killed
🎉`tech.picnic.errorprone.bugpatterns.JUnitClassModifiers`	0	6

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

werli

The rule itself LGTM, but I have one more question on one more of the test cases.

I'm optimistically approving however.

werli · 2022-12-04T10:25:46Z

...-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/JUnitClassModifiersTest.java

+            "  // BUG: Diagnostic contains:",
+            "  class NonFinal {",
+            "    @Test",
+            "    void foo() {}",
+            "  }",
+            "",
+            "  // BUG: Diagnostic contains:",
+            "  class NonFinalWithCustomTestMethod {",
+            "    @ParameterizedTest",
+            "    void foo() {}",
+            "  }",


I feel like these two cases are misleading: IIUC, the latter doesn't fail due to the "custom test method" but solely because it's non-final. Did I get this right?

Shouldn't we add a positive test case for test methods annotated with @ParameterizedTest? Though one could argue this is already covered by the test cases for MoreJUnitMatchers#TEST_METHOD in MoreJUnitMatchersTest.

Did I get this right?

Yes :)

Shouldn't we add a positive test case for test methods annotated with @ParameterizedTest?

Can do :)

Stephan202

Tnx for the review @werli! Added one more commit.

Stephan202 · 2022-12-04T10:41:23Z

...-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/JUnitClassModifiersTest.java

+            "  // BUG: Diagnostic contains:",
+            "  class NonFinal {",
+            "    @Test",
+            "    void foo() {}",
+            "  }",
+            "",
+            "  // BUG: Diagnostic contains:",
+            "  class NonFinalWithCustomTestMethod {",
+            "    @ParameterizedTest",
+            "    void foo() {}",
+            "  }",


Did I get this right?

Yes :)

Shouldn't we add a positive test case for test methods annotated with @ParameterizedTest?

Can do :)

github-actions · 2022-12-04T10:46:56Z

Looks good. All 6 mutations in this change were killed.

class	surviving	killed
🎉`tech.picnic.errorprone.bugpatterns.JUnitClassModifiers`	0	6

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

rickie · 2022-12-04T15:48:35Z

Nice reviews and commits 🚀 !

oxkitsune requested review from Stephan202 and rickie August 24, 2022 08:09

oxkitsune force-pushed the gdejong/PSM-1571 branch from d49b98c to 3795ab5 Compare August 24, 2022 08:14

oxkitsune marked this pull request as draft August 25, 2022 09:24

oxkitsune force-pushed the gdejong/PSM-1571 branch from 3795ab5 to b469b47 Compare October 20, 2022 08:35

oxkitsune marked this pull request as ready for review October 21, 2022 08:57

rickie requested changes Oct 24, 2022

View reviewed changes

rickie added the new feature label Oct 24, 2022

rickie added this to the 0.5.0 milestone Oct 24, 2022

oxkitsune force-pushed the gdejong/PSM-1571 branch from 329300e to 2a995ad Compare October 25, 2022 07:31

rickie force-pushed the gdejong/PSM-1571 branch from 2a995ad to 2ee79fc Compare October 25, 2022 08:55

rickie reviewed Oct 25, 2022

View reviewed changes

rickie modified the milestones: 0.5.0, 0.6.0 Oct 30, 2022

oxkitsune changed the title ~~Introduce JUnitClassDeclaration~~ Introduce JUnitClassModifiers check Nov 1, 2022

oxkitsune requested a review from rickie November 1, 2022 09:31

oxkitsune force-pushed the gdejong/PSM-1571 branch from e9f8a42 to d2b4225 Compare November 1, 2022 09:33

rickie requested changes Nov 1, 2022

View reviewed changes

rickie reviewed Nov 1, 2022

View reviewed changes

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitClassModifiers.java Outdated Show resolved Hide resolved

eric-picnic mentioned this pull request Nov 4, 2022

Introduce More{ASTHelpers,JUnitMatchers,Matchers} utility classes #335

Merged

oxkitsune force-pushed the gdejong/PSM-1571 branch 2 times, most recently from 1c9cc15 to c6e7dab Compare November 11, 2022 13:00

rickie force-pushed the gdejong/PSM-1571 branch from c6e7dab to 7b99d50 Compare November 18, 2022 11:10

rickie approved these changes Nov 18, 2022

View reviewed changes

rickie reviewed Nov 18, 2022

View reviewed changes

oxkitsune force-pushed the gdejong/PSM-1571 branch 2 times, most recently from 9f1e0be to a512faa Compare November 22, 2022 12:43

rickie approved these changes Nov 23, 2022

View reviewed changes

Stephan202 force-pushed the gdejong/PSM-1571 branch from a512faa to ba9c96b Compare November 27, 2022 16:26

Stephan202 reviewed Nov 27, 2022

View reviewed changes

oxkitsune force-pushed the gdejong/PSM-1571 branch from 8b01f2f to 0efa54b Compare December 1, 2022 14:17

rickie reviewed Dec 2, 2022

View reviewed changes

oxkitsune and others added 6 commits December 3, 2022 09:38

Introduce JUnitClassDeclaration

48a6180

Use utilities introduced in #335

4c53bf0

Suggestions

aa167a2

Require spring boot tests to be package private

c9cf244

suggestions

8bce2e7

Suggestions

10f0900

Stephan202 force-pushed the gdejong/PSM-1571 branch from 4719126 to 10f0900 Compare December 3, 2022 09:25

Stephan202 approved these changes Dec 3, 2022

View reviewed changes

werli approved these changes Dec 4, 2022

View reviewed changes

One more test case

f5763ef

Stephan202 reviewed Dec 4, 2022

View reviewed changes

rickie merged commit 8803d23 into master Dec 4, 2022

rickie deleted the gdejong/PSM-1571 branch December 4, 2022 15:49

	link = BUG_PATTERNS_BASE_URL + "JUnitClassDeclaration",
	link = BUG_PATTERNS_BASE_URL + "JUnitClassModifiers",

	private static final MultiMatcher<ClassTree, AnnotationTree>
	private static final Matcher<ClassTree>

Introduce JUnitClassModifiers check #214

Introduce JUnitClassModifiers check #214

Uh oh!

Conversation

oxkitsune commented Aug 24, 2022 • edited by Stephan202 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rickie left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rickie left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rickie left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rickie left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

oxkitsune commented Nov 11, 2022

Uh oh!

rickie left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rickie left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rickie left a comment

Choose a reason for hiding this comment

Uh oh!

Stephan202 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Introduce `JUnitClassModifiers` check #214

Introduce `JUnitClassModifiers` check #214

oxkitsune commented Aug 24, 2022 •

edited by Stephan202

Loading

rickie left a comment •

edited

Loading